Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make numbered bookmarks work with numlock on #1093

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Davidy22
Copy link

As resolution to #1089, add a case so that plugin still works when num lock is on.

@Davidy22 Davidy22 changed the title Work with numlock on Numbered bookmarks work with numlock on Jul 21, 2021
@Davidy22 Davidy22 changed the title Numbered bookmarks work with numlock on Make numbered bookmarks work with numlock on Jul 21, 2021
@Davidy22
Copy link
Author

What's this about a julia lexxer failing the build? The build worked on my machine and I made the most innocent looking change, were there catastrophic side effects to a || I didn't know about?

@elextr
Copy link
Member

elextr commented Jul 21, 2021

The Geany-plugins CI is still using an olde distro to build on, Trusty, Geany itself has been updated to use a supported distro because of problems with Trusty, and it was upgraded to Bionic. (Xenial having reached end of standard life the month before the upgrade).

What has happened is that the new Julia filetype has leaked a C++17 object std::string_view from its Scintilla implementation, but thats ok with Bionic, and the versions used for nightlys, but not Trusty. Oh well C++17 was bound to leak at some point :-D

@frlan do you want to bring G-P CI distro up to the same standard as Geany?

@elextr
Copy link
Member

elextr commented Jul 21, 2021

@Davidy22 I would suggest that you use the official mask https://developer.gnome.org/gdk3/unstable/gdk3-Windows.html#GdkModifierType to remove all but the wanted modifiers, any of the others can change at any time in any version of GTK, or with any new keyboard with extra keys, so the current implementation is very fragile.

@Davidy22
Copy link
Author

Davidy22 commented Jul 21, 2021

Now that you mention it, should have figured there would be mask docs when numlock state was exactly 16 away, added

/* control+shift+number */
if(ev->state==5 || ev->state==21) {
if(ev->state & (GDK_CONTROL_MASK | GDK_SHIFT_MASK)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests for control or shift, not control and shift

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh word, that wasn't the way I thought it'd work. Thought it was checking right too when I was checking ctrl+number.

@eht16
Copy link
Member

eht16 commented Jul 21, 2021

The Geany-plugins CI is still using an olde distro to build on, Trusty, Geany itself has been updated to use a supported distro because of problems with Trusty, and it was upgraded to Bionic. (Xenial having reached end of standard life the month before the upgrade).

What has happened is that the new Julia filetype has leaked a C++17 object std::string_view from its Scintilla implementation, but thats ok with Bionic, and the versions used for nightlys, but not Trusty. Oh well C++17 was bound to leak at some point :-D

@frlan do you want to bring G-P CI distro up to the same standard as Geany?

/me is not @frlan (😄) but anyway: #1094

@elextr
Copy link
Member

elextr commented Jul 22, 2021

Just a note that this plugin will only work for keyboards that are similar to US keyboards where the shift characters of the numbers match the majik numbers in the iShiftNumbers array ie )!@#$%^&*(.

Since this is a plugin such limitations can be accepted but perhaps the initialisation of that array could be changed to use character literals to be a bit more obvious.

@Davidy22
Copy link
Author

I actually thought the magic numbers in iShiftNumbers were the punctuation keys too when I first looked at it, but when I was writing the fix I noticed the chunk of code starting line 1503 that seems to be finding out on load what the numbers turn into when you hit shift, the Fraser fellow seems to have thought of that. Still a slightly obtuse magic number list though

@elextr
Copy link
Member

elextr commented Jul 22, 2021

Ok, the magic number list is only the fallback default and is British keyboard (" and £ not @ and #). The code you pointed to is a good try and probably good enough for most cases, but it does assume that the digits are level 0 and shift is level 1, which may not be the case on all keyboards, but probably few enough it won't be a problem very often.

@elextr
Copy link
Member

elextr commented Jul 24, 2021

@Davidy22 I tried to rerun CI after merging #1094, but although it said the request was queued it doesn't seem to have happened, maybe you could push some inconsequential change (to a comment maybe) to re-trigger it.

@Davidy22
Copy link
Author

Ey, it passed. Noticed a meaningful comment correction to make a commit out of.

@eht16
Copy link
Member

eht16 commented Aug 7, 2021

Looks good to me, tested and works (on my German keyboard) with and without Numlock turned on.

@elextr
Copy link
Member

elextr commented Aug 7, 2021

Can anybody with a French keyboard (at least I think its a French keyboard I'm remembering) with numbers on the top and symbols below check if it works, perhaps even though they are on top the numbers will still be level 0?

@andy5995
Copy link
Contributor

I just tried this plugin and I have the same problem. I found the open issue.

When I disable numlock the numbered bookmarks work. I have a Logitech K120 USB wired keyboard.

Thank you for working on this @Davidy22 . Does this patch need further testing before it's merged?

@elextr
Copy link
Member

elextr commented Feb 16, 2024

Does this patch need further testing before it's merged?

Yes with keyboards with non-US intl layout, or those could be ignored since nobody with one has bothered to test in two and a half years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants